Skip to content
This repository was archived by the owner on Jan 17, 2021. It is now read-only.

Use the username provided in GCP address to ssh #126

Merged
merged 2 commits into from
Aug 23, 2019

Conversation

SscSPs
Copy link
Contributor

@SscSPs SscSPs commented Jul 18, 2019

Earlier if the user did something like this:
localUser@localhost:~$ sshcode gcp:userName@InstanceName
it'd still be ssh as localUser@InstanceName , thats because only IP was returned and when that's the case, ssh tries to use the current username for remote machine,
This fixes that.
Tested on my machine(Manjaro Linux).

Signed-off-by: Sahil Soni [email protected]

@SscSPs SscSPs changed the title Return not the <IP>, but the <user@IP> for GCP instances Use the username provided in GCP address to ssh Jul 19, 2019
@deansheather
Copy link
Member

I don't have gcloud, so could you please explain how this PR makes parseGCPSSHCmd return a host with a username?

@SscSPs
Copy link
Contributor Author

SscSPs commented Jul 22, 2019

The dryRumCmd( gcloud compute ssh --dry-run InstanceName ) (Line 538) return the command used to ssh to the instance directly(without gcluod sdk).
So, the command:
localUser@localhost:~$ gcloud compute ssh --dry-run myInstance
returns something like(comment mentioning it at line 553) :
ssh **ssh flags** [email protected]

Now this works good if we(sshcode) wanna use the current localUser to ssh, so if we use
localUser@localhost:~$ gcloud compute ssh --dry-run remoteUser@myInstance
returns something like :
ssh **ssh flags** [email protected]

Before my commit, we were ignoring this username (comment mentioning it at line 556), by returning the userIP, we retain the username provided by the user and ssh to the correct user on the instance.

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 555 to 565 should be removed since they are unneccessary. Other than that, LGTM.

@SscSPs
Copy link
Contributor Author

SscSPs commented Jul 26, 2019

Ah, thanks for pointing that out, I was reluctant on remove that part becuase it was checking that the IP Address was valid, your comment made me to do some digging, and yes, it NEEDS to be gone, its not just unnecessory, but a bug on its own.
I found out that when an external IP is not assigned to the GCP instance we are trying to ssh to, the dry-run command returns something like this:
ssh **sshFlags** sscsps@compute.{{instanceIDAtGCP}}
As we can see that the last part is not a valid IP and will thus fail that check, but this command is valid(well, there's a bug in gcloud sdk at this point that gives invalid sshFlags at this point in a certain case, will be reporting that to google soon, not throughly tested yet), we can ssh with the dry-run command provided(when corrected),
So, yeah, we should remove that.

Will push the change soon and add this to first message.

@SscSPs
Copy link
Contributor Author

SscSPs commented Jul 26, 2019

Ok, so I was testing my changes out for instances with no external IP, and I found that instances with no external IP addresses cant connect to internet.
This means downloading the code-server on the instance is not possible(at least i dont know how to do it). So I think we'll have to download it on our local machine and scp it over to the instance.
I think that is out of scope for this PR, will work on it and raise new PR when I do that.

Copy link
Member

@deansheather deansheather left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. These commits need to be signed with PGP before I can merge them though.

SscSPs added 2 commits July 27, 2019 16:53
This is not needed since dry-run will provide a working host, or will throw error anyways
@SscSPs
Copy link
Contributor Author

SscSPs commented Jul 27, 2019

Looks good to me. These commits need to be signed with PGP before I can merge them though.

Signed :)

@deansheather deansheather merged commit 940305f into coder:master Aug 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants